Skip to content

Comments

feat: add async discord notifier#56911

Merged
Lee-W merged 1 commit intoapache:mainfrom
dondaum:feat/async-discord-notifier-2
Nov 20, 2025
Merged

feat: add async discord notifier#56911
Lee-W merged 1 commit intoapache:mainfrom
dondaum:feat/async-discord-notifier-2

Conversation

@dondaum
Copy link
Contributor

@dondaum dondaum commented Oct 20, 2025

Add an asynchronous version of DiscordNotifier.

related: #55237

I sucessfully tested the DiscordNotifier with the following Dag:

from datetime import timedelta
from airflow import DAG
from airflow.sdk.definitions.deadline import AsyncCallback, DeadlineAlert, DeadlineReference
from airflow.providers.discord.notifications.discord import DiscordNotifier
from airflow.providers.standard.operators.empty import EmptyOperator
from airflow.sdk import task

with DAG(
    dag_id="deadline_discord_alert_example",
    deadline=DeadlineAlert(
        reference=DeadlineReference.DAGRUN_QUEUED_AT,
        interval=timedelta(seconds=20),
        callback=AsyncCallback(
            DiscordNotifier,
            kwargs={
                "text": "🚨 Dag {{ dag_run.dag_id }} missed deadline at {{ deadline.deadline_time }}. DagRun: {{ dag_run }}",
            },
        ),
    ),
):
    c = EmptyOperator(task_id="example_task")

    @task()
    def wait():
        import time
        time.sleep(60*5)

    
    c >> wait()
1
2

^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in airflow-core/newsfragments.

@dondaum dondaum force-pushed the feat/async-discord-notifier-2 branch 2 times, most recently from 2b61dc2 to 4394a97 Compare October 21, 2025 08:30
@dondaum dondaum force-pushed the feat/async-discord-notifier-2 branch 5 times, most recently from 1e45571 to 7e08be7 Compare October 21, 2025 18:48
@ferruzzi ferruzzi mentioned this pull request Oct 21, 2025
Closed
1 task
@ferruzzi
Copy link
Contributor

ferruzzi commented Oct 23, 2025

Looks like you have a handful of comments already, so I'll let things settle a bit before I review, but feel free to ping me when you are ready. As always, thanks for the work!

@Lee-W Lee-W marked this pull request as draft October 27, 2025 02:06
Lee-W
Lee-W reviewed Oct 27, 2025
View reviewed changes
Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need some work to make the CI green. Mark it as draft before it's ready to review again. Thanks!

@dondaum dondaum force-pushed the feat/async-discord-notifier-2 branch 2 times, most recently from a6ba5c0 to 94c90a4 Compare October 27, 2025 21:54
@dondaum
Copy link
Contributor Author

dondaum commented Oct 27, 2025

CI is green now.

In my opinion, it makes sense to continue with this feature if it looks good.

Yes, the backwards compatible part is redundant with #57143, but in my opinion we could add it to our open items list and change all providers to use the common-provider once #57143 is merged.

WDYT ? @ferruzzi @ramitkataria

@dondaum dondaum marked this pull request as ready for review October 27, 2025 22:22
@ferruzzi
Copy link
Contributor

ferruzzi commented Oct 27, 2025

we could add it to our open items list and change all providers to use the common-provider once #57143 is merged.

I'm not thrilled with how many todos we're leaving hanging, but I guess that's life. Resolve the comments that are resolved and I'll make a pass tomorrow.

@ferruzzi ferruzzi mentioned this pull request Oct 30, 2025
Merged
@ferruzzi
Copy link
Contributor

ferruzzi commented Nov 4, 2025 •
edited
Loading

Merged #57143, let's get this one wrapped now!

@dondaum - Let us know when you've cleaned it up and are ready for another round of reviews.

@Lee-W
Copy link
Member

Lee-W commented Nov 5, 2025

I'll mark it as draft. But please let us know when it's ready again. Looking forward to this one!

@Lee-W Lee-W marked this pull request as draft November 5, 2025 01:59
@dondaum dondaum force-pushed the feat/async-discord-notifier-2 branch from 94c90a4 to 0ffe1fc Compare November 5, 2025 19:29
@dondaum dondaum marked this pull request as ready for review November 5, 2025 19:30
@dondaum
Copy link
Contributor Author

dondaum commented Nov 5, 2025

we could add it to our open items list and change all providers to use the common-provider once #57143 is merged.

I'm not thrilled with how many todos we're leaving hanging, but I guess that's life. Resolve the comments that are resolved and I'll make a pass tomorrow.

Let's reduce our list of open items. I have opened a PR for the Slack provider for use airflow.providers.common.compat #57894

@dondaum
Copy link
Contributor Author

dondaum commented Nov 5, 2025

Thank you for all the patience and support. I'm opening it up for another round of reviews.

@ferruzzi
Copy link
Contributor

ferruzzi commented Nov 6, 2025

Let's reduce our list of open items. I have opened a PR for the Slack provider for use airflow.providers.common.compat #57894

To be clear, that wasn't a jab at you by any means. Just in case it came off the wrong way. I greatly appreciate your work on these and hope I didn't imply otherwise.

@dondaum dondaum force-pushed the feat/async-discord-notifier-2 branch from 0ffe1fc to 2de3d0c Compare November 6, 2025 09:10
@dondaum
Copy link
Contributor Author

dondaum commented Nov 6, 2025

Let's reduce our list of open items. I have opened a PR for the Slack provider for use airflow.providers.common.compat #57894

To be clear, that wasn't a jab at you by any means. Just in case it came off the wrong way. I greatly appreciate your work on these and hope I didn't imply otherwise.

Don't worry, I got it right. I like keeping things clean and I consider myself to have an intrinsic motivation to clean up my own things.

Lee-W
Lee-W reviewed Nov 6, 2025
View reviewed changes
@dondaum dondaum force-pushed the feat/async-discord-notifier-2 branch 2 times, most recently from 0007227 to be0b3ce Compare November 6, 2025 10:08
@dondaum dondaum force-pushed the feat/async-discord-notifier-2 branch from be0b3ce to e349bef Compare November 18, 2025 09:44
@dondaum
Copy link
Contributor Author

dondaum commented Nov 18, 2025

Is there anything I can do to move forward with this PR?

@Lee-W
Copy link
Member

Lee-W commented Nov 18, 2025

Will take a look tomorrow.

Lee-W
Lee-W reviewed Nov 19, 2025
View reviewed changes
Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mostly good, a nit and a typo.

@Lee-W Lee-W requested a review from jason810496 November 19, 2025 09:43
@dondaum dondaum force-pushed the feat/async-discord-notifier-2 branch 2 times, most recently from c0ab5ec to 5598892 Compare November 19, 2025 09:53
@Lee-W
Copy link
Member

Lee-W commented Nov 19, 2025

Approve as the remaining one is a nitpick. @dondaum, please click resolve for the comments you think you've already resolved :)

@dondaum dondaum force-pushed the feat/async-discord-notifier-2 branch from 5598892 to ba67b84 Compare November 19, 2025 11:56
@Lee-W
Copy link
Member

Lee-W commented Nov 19, 2025

I'll keep it open for a few days so that other can take a look. But overall looks good! Thanks!

jason810496
jason810496 approved these changes Nov 19, 2025
View reviewed changes
Copy link
Member

@jason810496 jason810496 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! LGTM to me overall !

@dondaum dondaum force-pushed the feat/async-discord-notifier-2 branch from ba67b84 to 1f02827 Compare November 20, 2025 08:25
@dondaum dondaum force-pushed the feat/async-discord-notifier-2 branch from 1f02827 to 280265c Compare November 20, 2025 08:53
@Lee-W Lee-W merged commit bd05901 into apache:main Nov 20, 2025
94 checks passed
aaron-wolmutt pushed a commit to aaron-wolmutt/airflow that referenced this pull request Nov 20, 2025
@dondaum @aaron-wolmutt
feat: add async discord notifier (apache#56911)
a9720f8
@jscheffl jscheffl mentioned this pull request Nov 27, 2025
Closed
62 tasks
Copilot AI pushed a commit to jason810496/airflow that referenced this pull request Dec 5, 2025
@dondaum
feat: add async discord notifier (apache#56911)
23e277e
itayweb pushed a commit to itayweb/airflow that referenced this pull request Dec 6, 2025
@dondaum @itayweb
feat: add async discord notifier (apache#56911)
1357c6f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ferruzzi ferruzzi ferruzzi left review comments

@Lee-W Lee-W Lee-W approved these changes

@jason810496 jason810496 jason810496 approved these changes

+2 more reviewers

@KoviAnusha KoviAnusha KoviAnusha left review comments

@ramitkataria ramitkataria ramitkataria left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area:providers provider:discord

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dondaum @ferruzzi @Lee-W @KoviAnusha @ramitkataria @jason810496